Update invalid date error to return 400 by ritvibhatt · Pull Request #5344 · opensearch-project/sql

ritvibhatt · 2026-04-14T16:33:01Z

Description

Invalid dates were returning a 500 when query was pushed down, updates to return a 400

Related Issues

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
New PPL command checklist all confirmed.
API changes companion pull request created.
Commits are signed per the DCO using --signoff or -s.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Ritvi Bhatt <ribhatt@amazon.com>

github-actions · 2026-04-14T16:34:15Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes Sub-PR theme: Fix legacy SQL error handling to return 400 for invalid dates Relevant files: legacy/src/main/java/org/opensearch/sql/legacy/executor/format/ErrorMessageFactory.java legacy/src/main/java/org/opensearch/sql/legacy/executor/format/OpenSearchErrorMessage.java legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java Sub-PR theme: Fix PPL/OpenSearch error handling to return 400 for invalid dates Relevant files: opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/ErrorMessageFactory.java opensearch/src/main/java/org/opensearch/sql/opensearch/response/error/OpenSearchErrorMessage.java opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java
⚡ Recommended focus areas for review Status Override The original code used the OpenSearchException's own status code (via `exception.status().getStatus()`), but now the caller-provided `status` parameter is always used instead. This means if an OpenSearchException has its own meaningful HTTP status (e.g., 404 Not Found), it will be overridden by whatever status is passed in. Verify that all callers always pass the correct status, especially for non-400/500 cases. return new OpenSearchErrorMessage((OpenSearchException) e, status); } else if (unwrapCause(e) instanceof OpenSearchException) { OpenSearchException exception = (OpenSearchException) unwrapCause(e); return new OpenSearchErrorMessage(exception, status); Status Override Same concern as in the legacy module: the OpenSearchException's own status is no longer used; the caller-supplied `status` is always used. If the OpenSearch exception carries a meaningful status (e.g., 404), it will be silently replaced. Ensure all callers pass the correct status. return new OpenSearchErrorMessage((OpenSearchException) cause, status); Metrics Counting The new logic increments `PPL_FAILED_REQ_COUNT_CUS` only when `status == BAD_REQUEST`, but previously it was always incremented for any `OpenSearchException`. Non-400 OpenSearch exceptions (e.g., 503, 429) will now increment `PPL_FAILED_REQ_COUNT_SYS` instead of `PPL_FAILED_REQ_COUNT_CUS`. Verify this is the intended behavior change and that dashboards/alerts relying on these metrics are updated accordingly. if (status == BAD_REQUEST) { Metrics.getInstance() .getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_CUS) .increment(); } else { Metrics.getInstance() .getNumericalMetric(MetricName.PPL_FAILED_REQ_COUNT_SYS) .increment(); } Duplicate Logic The `SearchPhaseExecutionException` shard-failure inspection logic is duplicated between `RestPPLQueryAction` and `RestSqlAction`. Consider extracting this into a shared utility method to avoid maintenance burden and inconsistency. if (e instanceof SearchPhaseExecutionException) { for (ShardSearchFailure failure : ((SearchPhaseExecutionException) e).shardFailures()) { Throwable cause = failure.getCause(); if (cause instanceof Exception && isClientError((Exception) cause)) { status = BAD_REQUEST; break; } } } Error Wrapping Non-`VirtualMachineError` `Throwable`s (e.g., `Error` subclasses like `OutOfMemoryError` or `StackOverflowError` that are not `VirtualMachineError`) are now wrapped in a `RuntimeException` and passed to `listener.onFailure`. This may cause these serious JVM errors to be silently swallowed or reported as generic failures instead of propagating. Consider also rethrowing `Error` subclasses that are not `VirtualMachineError`. } catch (Throwable t) { if (t instanceof VirtualMachineError) { throw (VirtualMachineError) t; } Exception e = (t instanceof Exception) ? (Exception) t : new RuntimeException(t); listener.onFailure(e);

github-actions · 2026-04-14T16:34:48Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Rethrow all JVM errors, not just VirtualMachineError `VirtualMachineError` is a subclass of `Error`, but other serious `Error` types like `ThreadDeath` or `LinkageError` are also not safe to swallow. Consider rethrowing all `Error` instances, not just `VirtualMachineError`, to avoid masking critical JVM-level problems. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java [225-231] } catch (Throwable t) { - if (t instanceof VirtualMachineError) { - throw (VirtualMachineError) t; + if (t instanceof Error) { + throw (Error) t; } - Exception e = (t instanceof Exception) ? (Exception) t : new RuntimeException(t); - listener.onFailure(e); + listener.onFailure((Exception) t); } Suggestion importance[1-10]: 5 __ Why: Rethrowing all `Error` instances (not just `VirtualMachineError`) is a valid defensive practice to avoid masking critical JVM-level issues like `ThreadDeath` or `LinkageError`. However, the improved code also removes the wrapping of non-`Exception` `Throwable` types, which could cause a `ClassCastException` if a non-`Error`, non-`Exception` `Throwable` is thrown.	Low
General	Consistent cause-chain traversal for shard failures The `isClientError` method now traverses the cause chain, but for `SearchPhaseExecutionException` shard failures, only the direct cause is checked. If the relevant client error is wrapped deeper in the shard failure's cause chain, it won't be detected. Consider passing the shard failure cause to `isClientError` which already handles cause chain traversal, but also ensure the shard failure cause itself is checked recursively. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestPPLQueryAction.java [117-130] } else if (e instanceof OpenSearchException) { OpenSearchException exception = (OpenSearchException) e; RestStatus status = exception.status(); if (e instanceof SearchPhaseExecutionException) { + for (ShardSearchFailure failure : + ((SearchPhaseExecutionException) e).shardFailures()) { + Throwable cause = failure.getCause(); + // isClientError already traverses the cause chain + if (cause instanceof Exception && isClientError((Exception) cause)) { + status = BAD_REQUEST; + break; + } + } + } Suggestion importance[1-10]: 2 __ Why: The `isClientError` method already traverses the cause chain, so passing `failure.getCause()` to it already handles recursive checking. The suggestion's `improved_code` is essentially identical to the existing code, making this a non-improvement.	Low
Possible issue	Add null check before casting shard failure cause The `SearchPhaseExecutionException` check only looks at the first level of shard failure causes. If `shardFailures()` returns an empty array (null-safe concern), iterating is safe, but if `failure.getCause()` is null, passing null to `isClientError` could cause a NullPointerException. Add a null check before calling `isClientError`. legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java [177-186] if (exception instanceof SearchPhaseExecutionException) { for (ShardSearchFailure failure : ((SearchPhaseExecutionException) exception).shardFailures()) { Throwable cause = failure.getCause(); - if (cause instanceof Exception && isClientError((Exception) cause)) { + if (cause instanceof Exception && cause != null && isClientError((Exception) cause)) { status = BAD_REQUEST; break; } } } Suggestion importance[1-10]: 1 __ Why: The `cause instanceof Exception` check already implicitly handles the null case since `null instanceof Exception` evaluates to `false`, making the additional `cause != null` check redundant. The suggestion is technically incorrect in its premise.	Low

update invalid date error to return 400

42a3670

Signed-off-by: Ritvi Bhatt <ribhatt@amazon.com>

ritvibhatt closed this Apr 15, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update invalid date error to return 400#5344

Update invalid date error to return 400#5344
ritvibhatt wants to merge 1 commit into
opensearch-project:mainfrom
ritvibhatt:invalid-date-error-fix

ritvibhatt commented Apr 14, 2026

Uh oh!

github-actions Bot commented Apr 14, 2026

Uh oh!

github-actions Bot commented Apr 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ritvibhatt commented Apr 14, 2026

Description

Related Issues

Check List

Uh oh!

github-actions Bot commented Apr 14, 2026

PR Reviewer Guide 🔍

Uh oh!

github-actions Bot commented Apr 14, 2026

PR Code Suggestions ✨

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant